fix(vscode): use shell on Windows when spawning npm/ocr in CliService#490
Open
addielaruee wants to merge 1 commit into
Open
fix(vscode): use shell on Windows when spawning npm/ocr in CliService#490addielaruee wants to merge 1 commit into
addielaruee wants to merge 1 commit into
Conversation
On Windows, `node` is `node.exe` (spawnable directly) but `npm` and the `ocr` CLI are `.cmd` wrapper scripts, which Node's `child_process.spawn` cannot execute without `shell: true`. This made the extension's environment check false-report "npm not detected" even though `npm --version` works in every terminal, and would also stop `runRaw` from launching the ocr CLI to run a review. `probeCommand` (environment detection) and `runRaw` (review execution) were missing the `shell: process.platform === 'win32'` option that the `install` method already sets. Add it to both, matching `install`'s pattern. Behavior on macOS/Linux is unchanged (`shell` stays `false`). Add CliService.shell.test.ts, which mocks `child_process.spawn` to assert the shell option per platform, verifying the cross-platform fix without needing a live Windows environment. Fixes alibaba#453
|
|
Contributor
|
✅ OpenCodeReview: No comments generated. Looks good to me. |
Collaborator
|
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
On Windows,
nodeisnode.exe(whichchild_process.spawncan execute directly), butnpmand theocrCLI are.cmdwrapper scripts. Node'sspawncannot execute a.cmdfile unlessshell: trueis set. As reported in #453, this made the VS Code extension's environment-check page false-report "npm not detected" even thoughnpm --versionworks in every terminal. The same gap would also stoprunRawfrom launching theocrCLI to actually run a review on Windows.The
installmethod already setsshell: process.platform === 'win32', but the two other spawn call sites did not:probeCommand— used for thenode/npm/ocrenvironment detection.runRaw— used to run a review (andllm test).This PR adds
shell: process.platform === 'win32'to both, matchinginstall's existing pattern. On macOS/Linux the option evaluates tofalse, so behavior there is unchanged.Type of Change
How Has This Been Tested?
yarn testpasses locally inextensions/vscode(11 suites, 95 tests).CliService.shell.test.ts, which mockschild_process.spawnand asserts theshelloption per platform (win32→true, others →false) for bothprobeCommandandrunRaw. I confirmed the new tests fail when the fix is reverted, so they actually guard the behavior.tsc --noEmit -p tsconfig.extension.jsonandeslintpass (no new warnings).npm.cmdpath was verified by code inspection against the root cause in Windows 环境下vs code插件检测误报"未检测到 npm"（spawn 未使用 shell 模式） #453 and the already-correctinstallmethod, plus the platform-conditional unit tests above. Behavior on macOS/Linux is provably unchanged (shellstaysfalse).Checklist
Related Issues
Closes #453